Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add identifierItems for TaskGroupRenderSection #505

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add identifierItems for TaskGroupRenderSection #505

wants to merge 1 commit into from

Conversation

Kyle-Ye
Copy link
Contributor

@Kyle-Ye Kyle-Ye commented Mar 12, 2023 •
edited
Loading

Bug/issue #, if applicable:

Close #480

Summary

Update identifiers to identifierItems in TaskGroupRenderSection

## Overview

For more information see the [specification](https://example.com).

## See Also
- [Name 1](https://example.com)

- [Name 2](https://example.com)

- [Name 3](https://example3.com)

- [this great link ![random image](https://picsum.photos/200)](https://example3.com)

Before the PR

JSON output:

"seeAlsoSections": [
    {
        "title": "Related Documentation",
	    "identifiers": [
	      "https://example.com",
	      "https://example.com",
	      "https://example3.com",
	      "https://example3.com"
	    ]
    }
]

swift-docc-render:
image

After the PR

JSON output:

"seeAlsoSections": [
  {
    "title": "Related Documentation",
    "identifiers": [
      "https://example.com",
      "https://example.com",
      "https://example3.com",
      "https://example3.com"
    ],
    "identifierItems": [
      {
        "identifier": "https://example.com"
      },
      {
        "overrideTitle": "Name 2",
        "overridingTitleInlineContent": [
          {
            "type": "text",
            "text": "Name 2"
          }
        ],
        "identifier": "https://example.com"
      },
      {
        "identifier": "https://example3.com"
      },
      {
        "overrideTitle": "this great link ",
        "overridingTitleInlineContent": [
          {
            "type": "text",
            "text": "this great link "
          },
          {
            "type": "image",
            "identifier": "https://picsum.photos/200"
          }
        ],
        "identifier": "https://example3.com"
      }
    ]
  }
]

swift-docc-render:

Dependencies

Swift-DocC: #376 (Merged)
Swift-DocC-Render: swiftlang/swift-docc-render#691

Testing

[TODO]

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

  • Added tests
  • Ran the ./bin/test script and it succeeded
  • Updated documentation if necessary

@Kyle-Ye Kyle-Ye mentioned this pull request Mar 12, 2023
Open
2 tasks
@franklinsch
Copy link
Contributor

This approach makes sense to me. It would be good to align with the approach we've taken with the overridingTitleInlineContent parameter of RenderInlineContent.reference so that we support formatting in the title for extra flexibility.

@Kyle-Ye Kyle-Ye force-pushed the bugfix/kyle/see-also branch from 9c11d85 to 3181fbd Compare June 7, 2023 13:31
@Kyle-Ye Kyle-Ye force-pushed the bugfix/kyle/see-also branch from 3181fbd to 88bbf35 Compare June 7, 2023 15:19
@Kyle-Ye Kyle-Ye marked this pull request as ready for review June 7, 2023 15:19
@Kyle-Ye Kyle-Ye mentioned this pull request Jun 7, 2023
Open
3 tasks
@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Jun 7, 2023

This approach makes sense to me. It would be good to align with the approach we've taken with the overridingTitleInlineContent parameter of RenderInlineContent.reference so that we support formatting in the title for extra flexibility.

Update the swift-docc side implementation with your suggestion.

And the swift-docc-render PR is half-ready.

@mportiz08
Copy link
Contributor

I have a minor concern about the proposed Render JSON changes here. I like that the new identifierItems section was added as a way to maintain backwards compatibility for the new feature as opposed to modifying the existing identifiers array structure.

However, it seems that the new array will always be presented, even in the case where it isn't needed for the more common scenario where none of the links have custom titles. I'm slightly worried that this will add to the size of large libraries of content since the object with the duplicated identifier will be added for each see-also item.

Is there a way to make this an optional field that only gets included in Render JSON when it's needed for custom titles?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Impossible to put same link in body and see also with different title
3 participants
@Kyle-Ye @franklinsch @mportiz08